It is not clear to me what is this document a spec of? This paragraph describes our implementation, not the spec of the protocol (which is implementation-agnostic). E.g. it is an implementation detail that we use kbucket data structure. Other places in our stack use XOR-tries instead (which is generally recommended). So, what is this a spec of? Or is this document a description of an implementation?

A protocol spec would be worded differently. It would say: An implementation of a Kademlia node must maintain K peers with shared prefix of length L, for every L.

So, what is this a spec of?

Good point. This should be a specification of the protocol. Not a description of a specific implementation.

E.g. it is an implementation detail that we use kbucket data structure.

Thanks. 9355a8f refines this section, treating the data structure as an implementation detail. The section now contains your generic description using key prefix lengths.

Other places in our stack use XOR-tries instead (which is generally recommended).

I was not aware of XOR-tries. I will take a deeper look. With 9355a8f the specification suggests XOR-tries as one possible data structure.

This is not a routing operation (as the section title suggests). Also the description is not accurate.
PUT_VALUE puts a value on the node it is called on. (Not on the "closest node to the key").

Changed via 072360f. Let me know what you think @petar.

Same comment as above.

Changed via 072360f. Let me know what you think @petar.

"Implementations may diverge from this base algorithm": so what's the point of describing this algorithm if our implementation is different and other implementations don't have to follow this?

The motivation for including this basic algorithm is to enable readers to write a minimal implementation compatible with the existing networks (e.g. IPFS and Polkadot) without having to read e.g. the go-libp2p or rust-libp2p source code.

That said, I don't feel strongly about this. If you prefer I am happy to remove it. In my opinion we should add descriptions of the more advanced algorithms from go-libp2p in future pull requests.

Sounds good. I would just elaborate slightly, something like: "may diverge as long as they adhere to the wire format and make progress towards the target key"

Added in b074091.

Our implementation, is seeded with K peers.

Thanks. Adjusted via c4d4b53.

In our implementation, a lookup actually terminates when a given percent (e.g. 80%) of the beta (another parameter) peers closest to the target have been contacted.

Good to know. That might be something worth exploring for rust-libp2p as well.

As mentioned above, I would suggest keeping this sample algorithm minimal for now, extending it with various optimization descriptions in future pull requests. Again, I don't feel strongly about this.

That sounds fine. What you've written will work. It'll just wait quite a bit before it is ready to terminate, but it is correct.

When do you do the picking? Our implementation does this after a prior request completes and the state is updated.

I am not sure I follow. Steps 1 - 4 are executed in a loop, thus picking happens at the very start as well as each time a response is received (3) and the termination criterion (1) is still false. As far as I can tell, this is inline with what you describe above. Does that make sense @petar?

You are right... I overlooked. This looks fine.

The motivation for including this basic algorithm is to enable readers to write a minimal implementation compatible with the existing networks (e.g. IPFS and Polkadot) without having to read e.g. the go-libp2p or rust-libp2p source code.

That said, I don't feel strongly about this. If you prefer I am happy to remove it. In my opinion we should add descriptions of the more advanced algorithms from go-libp2p in future pull requests.

Thanks. Adjusted via c4d4b53.

Good to know. That might be something worth exploring for rust-libp2p as well.

As mentioned above, I would suggest keeping this sample algorithm minimal for now, extending it with various optimization descriptions in future pull requests. Again, I don't feel strongly about this.

I am not sure I follow. Steps 1 - 4 are executed in a loop, thus picking happens at the very start as well as each time a response is received (3) and the termination criterion (1) is still false. As far as I can tell, this is inline with what you describe above. Does that make sense @petar?

So, what is this a spec of?

Good point. This should be a specification of the protocol. Not a description of a specific implementation.

E.g. it is an implementation detail that we use kbucket data structure.

Thanks. 9355a8f refines this section, treating the data structure as an implementation detail. The section now contains your generic description using key prefix lengths.

Other places in our stack use XOR-tries instead (which is generally recommended).

I was not aware of XOR-tries. I will take a deeper look. With 9355a8f the specification suggests XOR-tries as one possible data structure.

Changed via 072360f. Let me know what you think @petar.

Changed via 072360f. Let me know what you think @petar.

Not sure if worth pointing out here that GET_PROVIDERS and GET_VALUE both have FIND_NODE semantics too (i.e. they return values and/or closest peers to the target) and so we can optimize by not needing to do FIND_NODE's first.

Not sure if worth pointing out here that GET_PROVIDERS and GET_VALUE both have FIND_NODE semantics too (i.e. they return values and/or closest peers to the target)

This is detailed in the corresponding GET_PROVIDER and GET_VALUE message descriptions in ## RPC messages:

* `GET_VALUE`: In the request `key` is an unstructured array of bytes. If `key`
  is a public key (begins with `/pk/`) and the key is known, the response has
  `record` set to that key. Otherwise, `record` is set to the value for the
  given key (if found in the datastore) and `closerPeers` is set to the `k`
  closest peers.

* `GET_PROVIDERS`: In the request `key` is set to a CID. The target node
  returns the closest known `providerPeers` (if any) and the `k` closest known
  `closerPeers`.

What do you think @aschmahmann? Is this good enough?

and so we can optimize by not needing to do FIND_NODE's first.

I am not quite sure why one should do a FIND_NODE before a GET_PROVIDER or GET_VALUE in the first place? As far as I know this is neither suggested in the Kademlia paper nor in this specification (e.g. see #### Content provider discovery section). Am I missing something @aschmahmann?

Is this good enough?

Probably?

Am I missing something @aschmahmann?

Nope, I guess it's just an obvious sort of thing. It could have been designed that GET_VALUE only returned the value/error instead of also sending back closest peers but this way is more efficient.

I'd be a bit careful with the wording here. FIND_NODE is unfortunately an overloaded function that does two things:

1. Tells you the closest DHT server peers to the target
2. Tells you about a peer you explicitly asked for even if they are not a DHT server

The second thing is frequently referred to as peer routing since it's helping route you to a peer. I'm not sure what to call the first, but it's routing you towards the target key in the peer-space (not quite the kademlia/xor space since the peerIDs need to be SHA256'd first).

Maybe just call this section FIND_NODE and define both behaviors.

If your curious this issue (libp2p/go-libp2p-kad-dht#584) describes my frustration with special casing around FIND_NODE and ADD/GET_PROVIDERS

I'd be a bit careful with the wording here. FIND_NODE is unfortunately an overloaded function that does two things:

1. Tells you the closest DHT server peers to the target

2. Tells you about a peer you explicitly asked for even if they are not a DHT server

Let me paraphrase the above to make sure I correctly understand your argument:

• On FIND_NODE a DHT server returns the closest other DHT servers to the given target.
• On FIND_NODE a DHT server returns a single DHT client if and only if the DHT client's peer ID matches the target.

In my eyes documenting this distinction without documenting the DHT client / server mode extension (see your comment on rust-libp2p) would be inconsistent.

I would prefer to tackle both in follow-up pull requests. What do you think @aschmahmann?

I added your comment to the list at the top of the pull request to keep track of it.

On FIND_NODE a DHT server returns a single DHT client if and only if the DHT client's peer ID matches the target.

Not quite, if we happen to know about a peer whose peerID is an exact match for the FIND_NODE query key then we add them to our list of servers to return. https://github.com/libp2p/go-libp2p-kad-dht/blob/6fff2a3b391f73da7a1c7558e27725ebe730e7ba/handlers.go#L256

In my eyes documenting this distinction without documenting the DHT client / server mode extension (see your comment on rust-libp2p) would be inconsistent.

I guess that's fair. If all nodes were servers this distinction wouldn't really matter. A follow up PR is fine.

Note: client mode isn't just for unreachable peers, it's generally recommended for "low quality" peers. This could mean low power, low bandwidth, low network uptime, etc. Basically peers that if they were servers would do more harm then good probably shouldn't be servers but may still want to make queries.

Why are we referencing the /pk space as part of the libp2p spec, is this just an example?

Two things:

1. This namespace shouldn't be required
2. It's effectively deprecated in IPFS

For context the main reason why it was deprecated was that it straddled this weird space between:

• Your key is too big to include in whatever other system actually needs your key
• Your key is small enough a DHT server will store it for you

Eventually IPFS decided that the distance between those two was effectively zero and started including the keys that were previously deemed too large (i.e. RSA) in IPNS records (the place they needed them). They also switched to Ed25519 keys by default since they're much smaller so they don't even need to embed them anymore.

Thanks both for the hint and the background. Removed with a065aac.

The point here is basically to determine if/when it's safe to shortcut your query and there are multiple ways to determine reliability.

A plain quorum is just one way. For example, you could also choose to adjust your view based on how close to the target the peer who gave you the content is.

I am in favor for this specification to be extended with more advanced procedures, though I would like to keep it simple in this iteration only mentioning the simple Quorum style. Is this ok for you @aschmahmann? If so, would you still like the wording to be adjusted?

I liked your wording "Implementations may diverge from this base algorithm as long as they adhere
to the wire format and make progress towards the target key." in that it indicated what we care about vs what we don't.

If you think it's obvious that all of this quorum stuff is optional client behavior and people can do whatever they want then feel free to leave it as is. If you think it's not really obvious, then it might be worth adding some wording around how when getting values you can abort whenever you feel confident you have the latest value, for example using a quorum.

324f915 extends the section, stressing the fact that quorum is one mechanism among many to determine when a query is finished. Let me know what you think.

You probably also want to send the PUT_VALUE to the closest peers who should've had it. i.e. you're sending to closest peers - peers who already have the latest

Great catch, thanks! Adjusted in 20b3b73.

This is generic to all Puts/Gets. Your implementation can do whatever as long as you only return valid data.

Your network may have implied and/or explicit expirations times per record type.

This is an artifact of past versions of this pull request. Thinking about it some more, I am in favor of removing it. To me it is implementation specific. 1dcb218 removes it.

@aschmahmann let me know if you disagree.

This isn't what go-libp2p does. We can slap on the usual "here's one way to do it disclaimer".

Unfortunately, this way is kind of bad because the bigger the network is the harder it is to find peers closer to you like this. At the very least you want to search for yourself in the network.

I can give the brief approach taken in go-libp2p-kad-dht if you're interested.

👍 c755a41 includes self lookup.

This isn't what go-libp2p does.

Neither does rust-libp2p. It first does a lookup for the local key and then tries to probabilistically generate a key for each bucket.

I can give the brief approach taken in go-libp2p-kad-dht if you're interested.

Yes.

Any reason to keep these in, maybe we should just denote the numbers as reserved/dead?

I kept them in to give some background on what these numbers were originally used for. In my eyes, with the note at the top // Note: These fields were removed from the Record message this shouldn't confuse anyone.

Not a strong opinion. Would you prefer this to be changed?

Your call. Was thinking it might make the long spec a little shorter was all.

What do you by exact match here?

As in the ID of the peer matches the target key in the original FIND_NODE request. Does that make sense @aschmahmann?

I'm not really sure why we don't always return the k closest peers but by https://github.com/libp2p/go-libp2p-kad-dht/blob/6fff2a3b391f73da7a1c7558e27725ebe730e7ba/handlers.go#L256

the only time we return a single peer is when Peer A asks Peer B "do you know peer B" and they say "yes, it's me". If Peer A asks Peer B "do you know peer C" they will say "yes here's there address along with k peers closer to them"

Thanks for looking this up. dab4549 simplifies the specification, always returning the k closest peers. I don't think we should document / require the peculiarity of go-libp2p-kad-dht returning a single peer when oneself was looked up. Sounds good?

I think that's right, or if it's important we can catch it in the follow up PR. We should investigate historical context here and see if a change is merited in go-libp2p-kad-dht.